Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RSK ledger integration #2577

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ahsan-javaiid
Copy link
Contributor

@ahsan-javaiid ahsan-javaiid commented Nov 8, 2022

Description

  • This PR adds ledger integration for RSK
  • Currently (main branch) is using hardcoded derivation path here in ledger service.
  • When ledger service connect with RSK app on ledger with eth dpath then RSK app will simply reject it.
  • So mainly this PR is making derivation path dynamic in ledger service and on UI as well to make it work for RSK

Demo

Screenshot 2022-11-08 at 6 38 35 PM

Type of Change

  • Fix/ Enhancement / Integration

Testing information

  • Activate SUPPORT_RSK env flag
  • Connect with ledger
  • Activate RSK app on ledger
  • Good to go
  • Transaction signing is working

Testing Environment

SUPPORT_RSK=true

Checklist

  • Git hooks enabled
  • No lint errors
  • Git commit is signed

Latest build: extension-builds-2577 (as of Mon, 20 Mar 2023 10:35:42 GMT).

@ahsan-javaiid
Copy link
Contributor Author

cc: @0xDaedalus @alepc253

Copy link
Contributor

@mhluongo mhluongo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From @zondax/ledger-icp's docs...

Please:

  • Do not use in production
  • Do not use a Ledger device with funds for development purposes.
  • Have a separate and marked device that is used ONLY for development and testing

We should aim to do this without additional outside dependencies, especially not experimental dependencies.

@@ -167,7 +168,10 @@ function LedgerAccountList({
{balance === null && <div className="balance_loading" />}
{balance !== null && (
<div className="balance">
{balance} {selectedNetwork.baseAsset.symbol}
{balance}{" "}
{appInfo?.appName === "RSK"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the approach we want here, otherwise we'd do the same thing for Polygon.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead, we should make sure that the selected network and Ledger app match. If they don't, we should ask the user if they'd like to switch to the RSK network... or tell them to switch Ledger apps (to either Ethereum or Polygon, based on the selected network).

This edge case probably needs some design thinking @VladUXUI

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can i get a little bit more context here?
Is this part of the onboarding or something else? asking because in onboarding we don't currently have option for users to change networks (although it's on the way).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for context - Polygon app on Ledger is really bad (and seems to be just a fork/extension of Ethereum app), was throwing weird errors and it was just a more stable approach to encourage users to use the Ethereum app for both ETH and Polygon. Maybe Rsk app is more stable.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jagodarybacka the problem is that you have to use the right app with the right network because the Ledger app only works with the right derivation path (ETH/Polygon and RSK have different dpath)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that we do not have any UI to select network/derivation path when user clicks on "connect to ledger" button. Tally wallet is assuming ethereum network by default here and attempt the connect with ledger using ethereum derivation path. In case of other networks like RSK connection get rejected by ledger app (RSK in this case) because tally wallet is passing hardcoded derivation path here

I can think of below two approaches to solve this issue:
Approach 1: Auto detect which app is running on device and use dPath w.r.t that app
Like in this PR i am auto detecting the active app on ledger and then passing dPath dynamically according to connected app.

Approach 2: Provide a UI to select network or derivation path before connecting to ledger so that user can select right network according to running app on ledger device.

So when user clicks on "connect to ledger" button, then user will be taken to derivation path selection screen instead of directly connecting to ledger as its doing right now and assuming ethereum derivation path

IMO, second approach is better and i can update the pull request with this approach.

Most welcome in case of any other thought's 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provide a UI to select network or derivation path before connecting to ledger

@VladUXUI I remember you were thinking about something similar before right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up question both for @alepc253 and @mr-michael - Are we OK turning on RSK without this PR being merged?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0xDaedalus yes, we are OK. This feature is important but can be supported in next releases. Launching RSK asap is the main goal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready for review.

@0xDaedalus 0xDaedalus added this to the RSK Support milestone Nov 11, 2022
@ahsan-javaiid ahsan-javaiid force-pushed the fix/rsk/ledger branch 2 times, most recently from fc7fabf to f6ccb1a Compare November 14, 2022 12:08
@ahsan-javaiid
Copy link
Contributor Author

Tally Team 👋

I have updated the pull request with ui based approach. Now before connecting to ledger user will be able to select dPath w.r.t ledger app.

See Demo video here:

Untitled.mp4
  • Also resolved previous feedback and not using any dependencies
  • Ready for review and most welcome for any thoughts 🤔

@@ -85,7 +85,7 @@ export const TEST_NETWORK_BY_CHAIN_ID = new Set(
[GOERLI].map((network) => network.chainID)
)

export const NETWORK_FOR_LEDGER_SIGNING = [ETHEREUM, POLYGON]
export const NETWORK_FOR_LEDGER_SIGNING = [ETHEREUM, POLYGON, RSK]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have renamed RSK > ROOTSTOCK in #2625
and this breaks the build and also the tests.

Sorry for this issue!

Are you ok fixing this, or would you prefer me to do it?

The fix patch is attached.
RSKtoROOTSTOCK.patch.txt

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -23,6 +23,7 @@ export default (prevState: Record<string, unknown>): NewState => {
currentDeviceID: null,
devices: {},
usbDeviceCount: 0,
derivationPath: null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not necessary but ❤️ the details

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@VladUXUI
Copy link
Contributor

This is looking awesome! 👏 😍
We will probably update soon with new onboarding, but i'm happy for this to be shipped as is (ui/ux wise)

@ahsan-javaiid ahsan-javaiid force-pushed the fix/rsk/ledger branch 4 times, most recently from 81abd08 to d69c094 Compare November 15, 2022 17:02
@ahsan-javaiid
Copy link
Contributor Author

Hi Tally team 👋

Feel free to share if there are any further thoughts on this PR.

Comment on lines 12 to 17
const DERIVATION_PATH_TO_NETWORK: { [key: string]: EVMNetwork } = {
"m/44'/137'/0'/0": ROOTSTOCK,
"m/44'/60'/0'/0": ETHEREUM,
"m/44'/1'/0'/0": ETHEREUM,
"m/44'/60'/0'": ETHEREUM,
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like the ledger lib should have some way to resolve this somewhere...

Copy link
Contributor Author

@ahsan-javaiid ahsan-javaiid Nov 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion @Shadowfiend. Purpose of this mapping is to get the right network against the derivation path that is returned by derivation path dropdown. Here i am setting the network selected by the user.
Objective here is to set the right network based on derivation path selected by user.

Feels like the ledger lib should have some way to resolve this somewhere...

I looked into tally codebase (ledger related files) to find something as you suggested. But i think such mapping or something related is not there unless there is some other intelligent way of achieving this.

Would be great if you can share some hint 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like the ledger lib should have some way to resolve this somewhere...

@Shadowfiend Resolved this concern

@ahsan-javaiid
Copy link
Contributor Author

Hi Tally team 👋

Updated this pull request and implemented the ledger upgrade flow to include dropdown

Reference issue: #2623

Demo:

Screenshot 2022-12-06 at 11 13 46 AM

Demo video:

ledgerdemo.mov

Feel free to review the pull request and share thought's if anything needs to changed. 🙏

Copy link
Contributor

@kkosiorowska kkosiorowska left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments on the code.

@VladUXUI Could you check the UI part? The solution is a little different from the design

background/constants/networks.ts Outdated Show resolved Hide resolved
background/main.ts Outdated Show resolved Hide resolved
background/redux-slices/ledger.ts Outdated Show resolved Hide resolved
ui/pages/Ledger/LedgerPrepare.tsx Outdated Show resolved Hide resolved
ui/pages/Ledger/LedgerSelectNetwork.tsx Outdated Show resolved Hide resolved
ui/components/LedgerMenu/LedgerMenuListItemChild.tsx Outdated Show resolved Hide resolved
Comment on lines 13 to 14
const DEFAULT_DERIVATION_PATH = "44'/60'/0'/0/0"
const ROOTSTOCK_DERIVATION_PATH = "44'/137'/0'/0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We declare these values in several places. Let's make it const so that if there is a change, it takes place in one place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a suggestion to make it part of network. Let me know if this idea makes sense or should i move it somewhere else?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I think it can be a part of the network but probably we don't want to mix this thing. Let's wait for @0xDaedalus opinion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have the derivation path on the Rootstock const 😄 For the default one I think we shouldn't just add it as a global const and use where needed and as a fallback.

ui/components/LedgerMenu/LedgerMenuProtocolListItem.tsx Outdated Show resolved Hide resolved
ui/components/LedgerMenu/LedgerMenuProtocolListItem.tsx Outdated Show resolved Hide resolved
ui/components/LedgerMenu/LedgerMenuProtocolListItem.tsx Outdated Show resolved Hide resolved
@ahsan-javaiid ahsan-javaiid force-pushed the fix/rsk/ledger branch 2 times, most recently from fdd260a to 0cb729d Compare December 8, 2022 13:24
@VladUXUI
Copy link
Contributor

This is looking good!
Will build today and test it out, as i see some small UI clean-up needed.
We since added BSC and Avalanche, so this will probably need to be updated a bit.
The logic is quite simple i hope, if it has a Ledger app, then it need a different select

@VladUXUI
Copy link
Contributor

Hey hey, so i just tested out the network selector from a ui pov, and added a few clean-up things. Pasting this as an image as it was easier for me to do.
image

Also i moved the Figma to a public file so you can access it and look at stuff.
https://www.figma.com/file/EiXpBpNz2HcoeFJftgC5oc/Tally-Public?node-id=1209%3A2200

Do let me know if it isn't working

@VladUXUI
Copy link
Contributor

Looking very good on my end! Awesome job @ahsan-javaiid!
Will let somebody from dev team to give final say

@kkosiorowska
Copy link
Contributor

It looks great after the design improvements! 😀 But I have a small problem with the AVAX network. I cannot connect by selecting this network. I don't have this issue for RSK or BNB chain.

Screenshot 2022-12-15 at 09 20 13

Copy link
Contributor

@kkosiorowska kkosiorowska left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're really close 🚀 I left a few tiny comments.

Let's still wait for @0xDaedalus final feedback. 😀
I wonder about one more thing. BSC and AVAX have feature flags. In this case, should we add the ability to switch off/on these networks?

background/redux-slices/ledger.ts Outdated Show resolved Hide resolved
ui/components/LedgerMenu/LedgerMenuProtocolList.tsx Outdated Show resolved Hide resolved
ui/components/LedgerMenu/LedgerMenuProtocolListItem.tsx Outdated Show resolved Hide resolved
Comment on lines 13 to 14
const DEFAULT_DERIVATION_PATH = "44'/60'/0'/0/0"
const ROOTSTOCK_DERIVATION_PATH = "44'/137'/0'/0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I think it can be a part of the network but probably we don't want to mix this thing. Let's wait for @0xDaedalus opinion.

@ahsan-javaiid
Copy link
Contributor Author

Hi Tally team 👋

Users are eager to use Tally wallet with Roostock <> Ledger. I would be happy to do any further changes to get this pull request considered.

Feel free to share further thoughts on this pull request.

@alepc253
Copy link

alepc253 commented Feb 7, 2023

hey @kkosiorowska @jagodarybacka, any update on this feature? is there anything else we could help with from Rootstock team?

Copy link
Contributor

@jagodarybacka jagodarybacka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay 😭 Awesome job was done here!

From UI testing:

  • I think this selector should be hidden for now as it is bugged and is not really changing dpath but is going back to network selector sceen
    image
  • with tabbed onboarding turned on SUPPORT_TABBED_ONBOARDING=true we should support a similar flow 🤔 but it shouldn't block this PR IMO

A couple of comments, mostly non-blocking, besides hiding bugged dpath selector

ui/_locales/en/messages.json Outdated Show resolved Hide resolved
ui/components/LedgerMenu/EcosystemNetworkIcon.tsx Outdated Show resolved Hide resolved
Comment on lines 13 to 14
const DEFAULT_DERIVATION_PATH = "44'/60'/0'/0/0"
const ROOTSTOCK_DERIVATION_PATH = "44'/137'/0'/0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have the derivation path on the Rootstock const 😄 For the default one I think we shouldn't just add it as a global const and use where needed and as a fallback.

background/services/ledger/index.ts Outdated Show resolved Hide resolved
background/services/ledger/index.ts Outdated Show resolved Hide resolved
ui/components/LedgerMenu/LedgerMenuProtocolListItem.tsx Outdated Show resolved Hide resolved
@jagodarybacka jagodarybacka dismissed mhluongo’s stale review February 8, 2023 13:45

fixed, no external dependencies

@ahsan-javaiid
Copy link
Contributor Author

Sorry for the delay 😭 Awesome job was done here!

From UI testing:

  • I think this selector should be hidden for now as it is bugged and is not really changing dpath but is going back to network selector sceen
    image
  • with tabbed onboarding turned on SUPPORT_TABBED_ONBOARDING=true we should support a similar flow 🤔 but it shouldn't block this PR IMO

A couple of comments, mostly non-blocking, besides hiding bugged dpath selector

Done!

@VladUXUI
Copy link
Contributor

With the addition of network selection at the beginning of the screen. Do we still need dpath selection?
Curious how many users have custom dpath in Ledger

@jagodarybacka
Copy link
Contributor

@VladUXUI I would say we do need it for the convenience of more advanced users, it would be a nice improvement here. For seed path import there is no way to choose the network there so we can either do it based on dpath selector or network selector 🤔

@ahsan-javaiid ty for fixes!

@0xDaedalus could you take a look - I think it's good to go and we can take care of tabbed onboarding later

@VladUXUI
Copy link
Contributor

Yeah, agreed. But i would rather fix than hide?
It looks like it's fixed

@ahsan-javaiid
Copy link
Contributor Author

Hi team: 👋

Just a reminder to consider this pull request.🎗 Feel free to share if anything blocking on this pull request. 🚧

Hope to get this PR considered soon. 🙏

@jagodarybacka
Copy link
Contributor

Hey @ahsan-javaiid I checked it yesterday and overall it looks nice. One thing I noticed we missed is that now we are missing Ledger live dpath when Ethereum is selected while importing Ledger. We will figure that out with @VladUXUI.

We are planning to release tabbed onboarding soon so we think its best to merge this PR after the initial release of tabbed onboarding is done 🔜
So no action is needed on your side right now, we will take care of it 🫶

@ahsan-javaiid
Copy link
Contributor Author

Hi Taho Team: 👀 Eyes on this pull request if it can be prioritised. 👀 🙏

@Shadowfiend
Copy link
Contributor

Hey folks, I wanted to poke at what is now #3385 before going deep on review here. Now that 3385 is written up, I'm going to try and give this some deep 👀 so we can get over the finish line 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants